-
Notifications
You must be signed in to change notification settings - Fork 18
Support for Knot.x setup in gradle aem multi #65
base: develop
Are you sure you want to change the base?
Conversation
Can you please check https://github.com/Knotx/knotx-starter-kit. It can be very useful when you consider Knot.x extensions development. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomaszmichalak all looks good, only one question to the code.
More general comment:
Please add section in README.md describing this Knot.X integration (with links to Knot.X documentation) - how it works and which endpoint contains page with data injected via knotx.
Without docs this whole feature is somewhat hidden.
aem/gradle/environment.gradle.kts
Outdated
@@ -27,6 +29,7 @@ configure<AemExtension> { | |||
url("Demo site", "http://demo.example.com/en-us.html", text = "English") | |||
url("Author login", "http://author.example.com/libs/granite/core/content/login.html" + | |||
"?resource=%2F&\$\$login\$\$=%24%24login%24%24&j_reason=unknown&j_reason_code=unknown", text = "AEM Sign In") | |||
url("Knot.x", "http://knotx.example.com/products/details.html", text = "Hello Knot.x with GAP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike comments / code looking like a poc. I more like looking like as much as production as it is possible
@@ -56,10 +56,11 @@ class PageModel : Serializable { | |||
} | |||
|
|||
private fun determineLanguageCode(): String { | |||
val languagePath = LanguageUtil.getLanguageRoot(resource.path) | |||
val languagePart = languagePath.substringAfterLast("/") | |||
// val languagePath = LanguageUtil.getLanguageRoot(resource.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why those lines are commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably code need to be only hardened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx guys, this PR now looks nice 😊 @Skejven the domain related problem is solved?
yes, it is solved in RC5 that we released today ;) |
after catch up with @tomaszmichalak agreeded TODO looks as below:
|
No description provided.